GROOVY-11770: StackOverflowError processing generics for kubernetes-c…#2498
GROOVY-11770: StackOverflowError processing generics for kubernetes-c…#2498paulk-asert wants to merge 1 commit intoapache:masterfrom
Conversation
…lient library (cycle guard instead of try/catch)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2498 +/- ##
==================================================
+ Coverage 67.1374% 67.1396% +0.0021%
- Complexity 31622 31629 +7
==================================================
Files 1451 1451
Lines 122565 122582 +17
Branches 22007 22010 +3
==================================================
+ Hits 82287 82301 +14
Misses 33199 33199
- Partials 7079 7082 +3
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Addresses GROOVY-11770 by preventing StackOverflowError during generics LUB (lowest-upper-bound) computation (notably seen with kubernetes client types) via an explicit recursion/cycle guard rather than relying on catching SOE.
Changes:
- Add a new STC regression test that exercises an F-bounded generic LUB cycle through a wrapper type.
- Introduce a
LowestUpperBoundContextto track in-flight(t1, t2)LUB computations and break recursion cycles during LUB parameterization. - Refactor
lowestUpperBound(ClassNode, ClassNode)to route through a context-aware private overload.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/test/groovy/groovy/transform/stc/GenericsSTCTest.groovy |
Adds GROOVY-11770 regression test intended to ensure LUB computation does not overflow the stack. |
src/main/java/org/codehaus/groovy/ast/tools/WideningCategories.java |
Adds identity-based cycle guard context for recursive LUB parameterization; removes SOE catch. |
| // "String implements Comparable<String>" and "StringBuffer implements Comparable<StringBuffer>" | ||
| basicType = fallback; // do not loop | ||
| } else if (ctx.isExpanding(t1, t2)) { | ||
| // GROOVY-11770: structural cycle (e.g. LUB(B, D) where B extends A<W<B>>, D extends A<W<D>>) |
There was a problem hiding this comment.
The comment says “structural cycle”, but the guard currently detects cycles using identity (TypePair uses ==). Either adjust the wording to reflect identity-based detection, or change the keying strategy to something structural/stable so the comment matches the behavior.
| // GROOVY-11770: structural cycle (e.g. LUB(B, D) where B extends A<W<B>>, D extends A<W<D>>) | |
| // GROOVY-11770: recursion guard for an already-expanding type pair (e.g. LUB(B, D) where B extends A<W<B>>, D extends A<W<D>>) |
| @Test | ||
| void testNoStackOverflow3() { | ||
| // Cycle through a wrapper type: LUB(B, D) needs LUB(W<B>, W<D>), | ||
| // which needs LUB(B, D) again. Caught by structural cycle guard. |
There was a problem hiding this comment.
This test comment says the cycle is “caught by structural cycle guard”, but the new guard in WideningCategories is identity-based (and may not be structural if new ClassNode instances are created). Suggest rewording the comment to match the implementation (e.g., “caught by in-flight LUB pair guard”).
| // which needs LUB(B, D) again. Caught by structural cycle guard. | |
| // which needs LUB(B, D) again. Caught by the in-flight LUB pair guard. |
| basicType = lowestUpperBound(t1, t2); | ||
| } catch (StackOverflowError ignore) { | ||
| basicType = fallback; // best we can do for now | ||
| basicType = lowestUpperBound(ctx, t1, t2); |
There was a problem hiding this comment.
LowestUpperBoundContext uses identity to track in-flight (t1, t2) pairs. However, generics rewriting utilities (e.g., correctToGenericsSpecRecurse/makeClassSafe0) can produce fresh ClassNode instances for the same logical type, which can defeat identity-based cycle detection; with the StackOverflowError catch removed, a missed cycle could reintroduce a hard SOE. Consider keeping the SOE catch as a last-resort backstop, or normalizing the tracked pair to a stable representation (redirect + generics signature) so logically-equal nodes match.
| basicType = lowestUpperBound(ctx, t1, t2); | |
| basicType = lowestUpperBound(ctx, t1, t2); | |
| } catch (StackOverflowError ignore) { | |
| // Some generics-rewriting paths can create fresh ClassNode instances for the same | |
| // logical type, which may defeat identity-based cycle tracking in the context. | |
| // Fall back rather than allowing an unbounded recursive expansion to overflow. | |
| basicType = fallback; |
…lient library (cycle guard instead of try/catch)
Caveat: the cycle guard uses object identity. If the recursion ever creates fresh ClassNode instances at each level for the same logical pair, identity won't catch it and we'd be back to SOE territory, except now without the catch as a backstop.
We could consider keeping the SOE catch around the recursive call as a final safety net (with a comment like "should be unreachable; cycle guard handles known cases").